-
Notifications
You must be signed in to change notification settings - Fork 1
홈 화면 및 GNB 구현 #39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
홈 화면 및 GNB 구현 #39
Conversation
📝 WalkthroughWalkthroughfeature/main 모듈이 새로 추가되어 settings.gradle.kts 및 app/build.gradle.kts에 포함·의존성이 등록되었습니다. core/ui에 MVU 스타일의 BaseViewModel, Intent/SideEffect 표식, Compose 확장(noRippleClickable, weekStartSunday)이 추가되었습니다. 네비게이션 식별자들이 Login→LoginRoute, HomeGraph→MainGraph, Home→MainRoute 등으로 재구성되고 AppNavHost의 시작 목적지가 MainGraph로 변경되었습니다. 다수의 벡터 드로어블과 문자열 리소스가 추가되었고 Kermit 로깅 라이브러리 및 twix.kermit 컨벤션 플러그인과 관련 Gradle 설정이 도입되었습니다. feature/main에는 Home 및 Main의 UI, ViewModel, 상태·인텐트·사이드이펙트, DI 모듈, 네비게이션 그래프가 포함됩니다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@core/design-system/src/main/res/drawable/ic_drop_down_arrow.xml`:
- Around line 6-11: The path element in ic_drop_down_arrow.xml hardcodes
fillColor and strokeColor to `#464646` which breaks dark-theme contrast; update
the drawable to avoid fixed colors by replacing android:fillColor and
android:strokeColor with a theme attribute (e.g., ?attr/colorOnBackground or a
dedicated ?attr/colorIcon) or remove those attributes so the view can apply a
tint at runtime; locate the <path ... android:fillColor="#464646"
android:strokeColor="#464646" ... /> and swap to the theme attribute or null it,
and ensure consuming views call setImageTintList or use android:tint to apply
the correct themed color.
In `@core/design-system/src/main/res/drawable/ic_home_unselected.xml`:
- Around line 6-10: The path in ic_home_unselected.xml hardcodes
strokeColor="#171717" which breaks theme/dark-mode support; replace the
hardcoded android:strokeColor on the <path> with a color resource reference
(e.g., `@color/icon_default`) and add a colors.xml entry named icon_default in
core/design-system/src/main/res/values/colors.xml (and a corresponding
values-night/colors.xml for dark mode) so icons follow theme colors instead of
fixed hex values.
In `@core/design-system/src/main/res/values/strings.xml`:
- Line 19: Rename the malformed string resource key homt_empty_goal_guide to
home_empty_goal_guide in strings.xml and update all usages to match (for example
the reference in EmptyGoalGuide.kt), ensuring the string value remains unchanged
and any import/lookup (e.g., R.string.homt_empty_goal_guide) is replaced with
R.string.home_empty_goal_guide so resource lookups compile correctly.
In `@feature/main/src/main/java/com/twix/home/component/WeeklyCalendar.kt`:
- Around line 60-66: LaunchedEffect(days.first()) runs on first composition and
with dragSumPx initial 0f will call onUpdateVisibleDate(days.last()); guard the
effect so it only triggers after a user swipe by adding a check (e.g., require
dragSumPx != 0f or track a hasSwiped/initialized boolean remembered and set only
when a drag completes) before calling onUpdateVisibleDate; update the
LaunchedEffect containing LaunchedEffect(days.first()), dragSumPx, and
onUpdateVisibleDate to return early when no user swipe has occurred so initial
composition does not change visibleDate.
In `@feature/main/src/main/java/com/twix/main/component/MainBottomBar.kt`:
- Around line 68-94: The clickable area is currently applied only to the Image,
so the tab label (AppText) is not interactive; move the clickable modifier from
the Image to the parent Column (apply modifier.clickable(...) with the same
indication, interactionSource, and onClick that performs
haptic.performHapticFeedback(HapticFeedbackType.TextHandleMove) then calls
onClick()), and remove the clickable modifier from the Image so the entire
Column (icon + label) is tappable while preserving the original
interactionSource, haptic behavior, and onClick handler.
🧹 Nitpick comments (27)
core/design-system/src/main/res/drawable/ic_couple_unselected.xml (1)
9-10: 하드코딩된 색상 값이 다크 모드를 지원하지 않습니다.
#171717은 거의 검정에 가까운 색상으로, 다크 모드에서 어두운 배경 위에 표시될 경우 아이콘이 보이지 않을 수 있습니다. 디자인 시스템 가이드라인에 따라, 테마에 반응하는 색상 리소스를 참조하는 것이 좋습니다.예를 들어:
@color/icon_on_surface같은 색상 리소스를 정의하고 참조- 또는
?attr/colorOnSurface같은 테마 속성 사용다크 모드 지원 계획이 있다면 지금 색상 리소스로 분리해 두시면, 향후 테마 변경 시 일괄 적용이 훨씬 수월해집니다.
♻️ 개선 제안
core/design-system/src/main/res/values/colors.xml에 색상 정의:<color name="icon_default">#171717</color>다크 모드용
core/design-system/src/main/res/values-night/colors.xml:<color name="icon_default">#E0E0E0</color>그 후 drawable에서 참조:
- android:fillColor="#00000000" - android:strokeColor="#171717"/> + android:fillColor="@android:color/transparent" + android:strokeColor="@color/icon_default"/>core/design-system/src/main/res/drawable/ic_couple_selected.xml (1)
1-11: 아이콘 쌍의 구조가 잘 설계되었습니다.
ic_couple_unselected.xml과 동일한 pathData를 사용하면서fillColor만 다르게 설정하여 선택/미선택 상태를 표현한 점이 좋습니다. 이렇게 하면 두 상태 간 전환 시 아이콘 형태가 일관되게 유지됩니다.다만, 앞서 언급한 것처럼
#171717하드코딩 색상은 다크 모드 지원을 위해 색상 리소스로 분리하는 것을 권장드립니다. 두 파일 모두 동일한 색상을 사용하므로, 한 번 리소스로 정의해 두면 유지보수가 편리해집니다.,
core/ui/src/main/java/com/twix/ui/extension/NoRippleClickable.kt (1)
9-15: 리플 효과 제거 확장 함수가 깔끔하게 구현되었습니다.기본 구현은 정확하며,
indication = null과MutableInteractionSource를 활용한 패턴이 적절합니다.다만 코딩 가이드라인에서 요구하는 커스터마이징 포인트를 고려했을 때,
enabled와 접근성 관련 파라미터를 추가하면 다양한 사용 사례에서 재사용성이 높아질 것으로 보입니다. 예를 들어, 버튼이 비활성화된 상태나 스크린 리더를 위한 라벨이 필요한 경우에도 대응할 수 있습니다.♻️ 커스터마이징 포인트 확장 제안
`@Composable` fun Modifier.noRippleClickable( + enabled: Boolean = true, + onClickLabel: String? = null, onClick: () -> Unit, ): Modifier = clickable( indication = null, interactionSource = remember { MutableInteractionSource() }, + enabled = enabled, + onClickLabel = onClickLabel, onClick = onClick, )코딩 가이드라인 기준 - 커스터마이징 포인트가 적절한가?에 따른 제안입니다.
core/design-system/src/main/res/drawable/ic_alert.xml (1)
10-10: Dark Mode 지원을 위해 테마 속성 사용을 권장합니다.현재
#464646색상이 하드코딩되어 있어 Dark Mode에서 아이콘이 잘 보이지 않을 수 있습니다. 디자인 시스템 가이드라인에 따라 테마 속성을 참조하거나, colors.xml에 정의된 색상 리소스를 사용하면 라이트/다크 모드 전환 시 자동으로 적절한 색상이 적용됩니다.♻️ 테마 속성을 사용한 개선 예시
- android:strokeColor="#464646" + android:strokeColor="?attr/colorOnSurface"또는 colors.xml에 별도 색상 리소스를 정의하고 참조:
- android:strokeColor="#464646" + android:strokeColor="@color/icon_default"core/design-system/src/main/res/drawable/ic_plus.xml (1)
10-10: 흰색 하드코딩이 의도된 것인지 확인이 필요합니다.FAB(Floating Action Button) 등 특정 컴포넌트에서 항상 흰색 아이콘을 사용하는 것이 의도라면 현재 구현이 적절합니다. 다만, 다른 곳에서도 재사용될 가능성이 있다면
?attr/colorOnPrimary같은 테마 속성을 사용하는 것이 Dark Mode 대응에 유리합니다.현재 사용처(AddGoalButton)를 고려할 때 의도된 디자인으로 보이므로 확인 차원에서 코멘트 남깁니다.
core/design-system/src/main/res/drawable/ic_empty_goal_arrow.xml (1)
8-8: 데코레이션 요소도 테마 대응을 고려해주세요.Empty state 가이드에 사용되는 장식용 화살표이지만,
#C6C6C6색상이 하드코딩되어 있어 Dark Mode에서 배경과 대비가 낮아질 수 있습니다. colors.xml에 정의된 색상 리소스를 참조하면 테마 전환 시 자동으로 적절한 색상이 적용됩니다.♻️ 색상 리소스 참조 예시
- android:fillColor="#C6C6C6" + android:fillColor="@color/illustration_secondary"core/design-system/src/main/res/values/strings.xml (1)
1-21: 다국어 지원 구조를 확인해주세요.현재 기본
values/strings.xml에 한국어 문자열이 직접 포함되어 있습니다. 향후 다국어 지원을 고려한다면, 다음과 같은 구조가 권장됩니다:
values/strings.xml: 영어(기본값)values-ko/strings.xml: 한국어현재 앱이 한국어 전용이라면 문제없지만, 추후 영어 등 다른 언어 지원 계획이 있다면 구조 개선을 고려해보시는 것이 좋겠습니다.
core/design-system/src/main/res/drawable/ic_stats_unselected.xml (1)
10-10: Dark Mode 지원을 위해 테마 색상 참조를 고려해 보세요.현재
strokeColor가#171717로 하드코딩되어 있습니다. 이 색상은 Dark Mode에서 배경과 대비가 낮아 아이콘이 잘 보이지 않을 수 있습니다.
@color/리소스나 테마 속성(?attr/colorOnSurface등)을 참조하면 Dark Mode에서도 자동으로 적절한 색상이 적용됩니다. 다만, 디자인 시스템 전체 아이콘에서 동일한 방식을 사용하고 있다면, 일관성을 위해 현재 상태를 유지하고 추후 일괄 개선하는 방향도 괜찮습니다.이 부분은 팀의 디자인 시스템 정책에 따라 결정해 주세요.
feature/main/src/main/java/com/twix/home/model/DateItemUiModel.kt (1)
3-6: 간결하고 명확한 UI 모델입니다! 👍
dayOfMonth와isSelected만으로 날짜 아이템 표현에 필요한 최소한의 정보를 담고 있어 좋습니다.data class로 정의하여 불변성도 확보되어 있습니다.Compose에서 불필요한 recomposition을 방지하기 위해
@Immutable어노테이션을 추가하는 것도 고려해 볼 수 있습니다. 다만, 이 모델이 리스트 아이템으로만 사용되고equals()비교가 잘 동작한다면 현재 상태로도 충분합니다.선택적 개선안: `@Immutable` 추가
package com.twix.home.model +import androidx.compose.runtime.Immutable + +@Immutable data class DateItemUiModel( val dayOfMonth: Int, val isSelected: Boolean, )core/design-system/src/main/res/drawable/ic_empty_face.xml (1)
1-57: 일러스트레이션 아이콘이 잘 구성되어 있습니다.Empty 상태를 표현하는 귀여운 캐릭터 일러스트레이션입니다. 복잡한 경로들이 잘 조합되어 있네요.
Dark Mode 고려 사항:
현재#ffffff(흰색)와#464646(어두운 회색) 색상이 하드코딩되어 있습니다. Dark Mode에서는 흰색 배경이 너무 밝게 보이거나, 어두운 배경과 캐릭터의 대비가 어색해질 수 있습니다.해결 방안으로는:
res/drawable-night/폴더에 Dark Mode용 별도 아이콘 추가- 또는 색상을
@color/리소스로 분리하여 테마별 색상 적용이 부분은 디자인 가이드에 따라 결정해 주세요. 현재 앱이 Dark Mode를 지원하지 않는다면 나중에 개선해도 됩니다.
core/design-system/src/main/res/drawable/ic_stats_selected.xml (1)
8-14: 다크 모드 대응을 위해 색상 리소스 사용을 권장합니다.현재
#171717색상이 직접 하드코딩되어 있어 다크 모드에서 아이콘이 배경과 구분되지 않을 수 있습니다. 디자인 시스템의 일관성과 다크 모드 지원을 위해res/color에 정의된 색상 리소스를 참조하거나, 사용처에서tint를 동적으로 적용하는 방식을 고려해 주세요.예를 들어,
fillColor와strokeColor에?attr/colorOnSurface같은 테마 속성을 사용하면 테마 변경 시 자동으로 적절한 색상이 적용됩니다.feature/main/src/main/java/com/twix/home/component/HomeTopBar.kt (2)
58-63: contentDescription에 하드코딩된 문자열 대신 stringResource 사용을 권장합니다.현재
"drop down arrow","notification","setting"등의 contentDescription이 영어로 하드코딩되어 있습니다. 접근성(TalkBack) 사용자를 위해 다국어 지원이 필요하며, 리소스 파일에 정의된 문자열을 사용하는 것이 좋습니다.♻️ 개선 제안
Image( painter = painterResource(R.drawable.ic_drop_down_arrow), - contentDescription = "drop down arrow", + contentDescription = stringResource(R.string.content_desc_drop_down), modifier = Modifier.size(20.dp), colorFilter = ColorFilter.tint(GrayColor.C400), )
strings.xml에 다음과 같이 추가:<string name="content_desc_drop_down">월 선택 드롭다운</string> <string name="content_desc_notification">알림</string> <string name="content_desc_setting">설정</string>
105-109: 접근성을 위해 ripple indication 제거 여부를 검토해 주세요.
indication = null로 설정하면 터치 시 시각적 피드백이 사라집니다. 이는 시각 장애가 있는 사용자나 터치 확인이 필요한 사용자에게 불편할 수 있습니다.의도적으로 ripple을 제거한 것이라면 괜찮지만, 접근성 가이드라인에 따르면 인터랙티브 요소는 명확한 피드백을 제공해야 합니다. Material3의 기본 ripple을 유지하거나, 커스텀 indication을 제공하는 것을 고려해 보시겠어요?
♻️ 기본 ripple 유지 방안
Box( modifier = Modifier .size(40.dp) - .clickable( - indication = null, - interactionSource = interactionSource, - onClick = onClick, - ), + .clickable( + interactionSource = interactionSource, + indication = ripple(bounded = false, radius = 20.dp), + onClick = onClick, + ), contentAlignment = Alignment.Center, )feature/main/src/main/java/com/twix/home/component/EmptyGoalGuide.kt (2)
23-59: 재사용 가능한 Composable로 잘 설계되었습니다.
Modifier파라미터를 통해 외부에서 커스터마이징 가능하고, 순수 함수로 작성되어 있습니다. 몇 가지 개선 제안이 있습니다:1. Preview Composable 추가 필요
코딩 가이드라인에 따르면 다양한 상태에 대한 Preview가 제공되어야 합니다. 이 컴포넌트는 단일 상태이므로 기본 Preview만 추가해도 충분합니다.
2. contentDescription 국제화
"empty face","empty goal arrow"가 하드코딩되어 있습니다. 접근성을 위해 문자열 리소스로 추출하는 것이 좋습니다.3. 문자열 리소스 키 오타 확인
R.string.homt_empty_goal_guide에서homt가home의 오타인지 확인해 주세요.♻️ Preview 추가 예시
`@Preview`(showBackground = true) `@Composable` private fun EmptyGoalGuidePreview() { EmptyGoalGuide() }
32-38: 이미지 접근성을 위한 contentDescription 국제화 제안스크린 리더 사용자를 위해
contentDescription을 문자열 리소스로 관리하면 다국어 지원이 가능해집니다. 장식용 이미지라면null로 설정하여 스크린 리더가 건너뛰도록 할 수도 있습니다.♻️ 제안 코드
Image( painter = painterResource(R.drawable.ic_empty_face), - contentDescription = "empty face", + contentDescription = stringResource(R.string.empty_face_content_description), // 또는 장식용이라면: contentDescription = null, ... )Also applies to: 51-57
feature/main/src/main/java/com/twix/main/component/MainBottomBar.kt (1)
29-55: Preview Composable 추가를 고려해 주세요.코딩 가이드라인에 따르면 Preview Composable을 제공하는 것이 권장됩니다. 이를 통해 개발 중 빠른 UI 확인이 가능해집니다.
💡 Preview 추가 예시
`@Preview`(showBackground = true) `@Composable` private fun MainBottomBarPreview() { MainBottomBar( selectedTab = MainTab.HOME, onTabClick = {}, ) }feature/main/src/main/java/com/twix/home/model/HomeUiState.kt (1)
9-17: State 정의가 불변 데이터 클래스로 잘 구현되어 있습니다.
@Immutable어노테이션과data class를 사용하여 MVI 패턴의 State를 올바르게 정의하고 있습니다.몇 가지 확인 사항이 있습니다:
month속성의 용도 확인:monthYear가visibleDate에서 파생되고 있는데,month속성이 별도로 필요한 용도가 있을까요? 현재는 사용되지 않는 것처럼 보입니다.국제화(i18n) 고려:
monthYear의 "월" 문자열이 하드코딩되어 있습니다. 다국어 지원이 필요하다면 리소스 문자열로 분리하는 것을 고려해 주세요.feature/main/src/main/java/com/twix/main/MainScreen.kt (2)
41-62: Scaffold 배경색 설정 방식 개선 제안
Scaffold의modifier에.background(CommonColor.White)를 적용하고, 내부Box에도 동일한 배경색을 적용하고 있습니다.Scaffold는containerColor파라미터를 통해 배경색을 설정하는 것이 더 적절합니다.modifier.background()는Scaffold내부 컨텐츠 영역에 적용되지 않을 수 있습니다.♻️ 개선 제안
Scaffold( - modifier = - Modifier - .fillMaxSize() - .background(CommonColor.White), + modifier = Modifier.fillMaxSize(), + containerColor = CommonColor.White, bottomBar = { MainBottomBar( selectedTab = selectedTab, onTabClick = onTabClick, ) }, ) { padding -> Box( modifier = Modifier .fillMaxSize() - .background(CommonColor.White) .padding(padding), ) { content(selectedTab) } }
35-63: Preview Composable 미제공코딩 가이드라인에 따르면 Compose UI에는 Preview Composable이 제공되어야 합니다.
MainScreen에 대한 Preview 함수를 추가하면 UI 개발 및 검증이 용이해집니다.♻️ Preview 추가 예시
`@Preview`(showBackground = true) `@Composable` private fun MainScreenPreview() { MainScreen( selectedTab = MainTab.HOME, onTabClick = {}, content = { Box(modifier = Modifier.fillMaxSize()) } ) }feature/main/src/main/java/com/twix/home/HomeScreen.kt (3)
45-91: Preview Composable 미제공코딩 가이드라인에 따르면 다양한 상태에 대한 Preview가 제공되어야 합니다.
HomeScreen의 여러 상태(빈 목표, 목표 있음 등)에 대한 Preview를 추가하면 UI 검증이 용이해집니다.♻️ Preview 추가 예시
`@Preview`(showBackground = true) `@Composable` private fun HomeScreenPreview() { HomeScreen( uiState = HomeUiState(), onSelectDate = {}, onPreviousWeek = {}, onNextWeek = {}, onUpdateVisibleDate = {}, ) }
63-67: 빈 클릭 핸들러에 대한 TODO 주석 추가 권장
onNotificationClick과onSettingClick이 빈 람다로 전달되고 있습니다. 향후 구현 예정임을 명시하는 TODO 주석을 추가하면 코드 추적이 용이해집니다.♻️ 개선 제안
HomeTopBar( monthYearText = uiState.monthYear, - onNotificationClick = {}, - onSettingClick = {}, + onNotificationClick = { /* TODO: 알림 화면 연결 */ }, + onSettingClick = { /* TODO: 설정 화면 연결 */ }, )
110-116: contentDescription 국제화(i18n) 고려
contentDescription이 하드코딩된 영어 문자열("add goal")로 되어 있습니다. 접근성을 위해stringResource를 사용하여 국제화를 적용하는 것이 좋습니다.♻️ 개선 제안
Image( painter = painterResource(R.drawable.ic_plus), - contentDescription = "add goal", + contentDescription = stringResource(R.string.content_desc_add_goal), modifier = Modifier .size(40.dp), )feature/main/src/main/java/com/twix/home/component/WeeklyCalendar.kt (3)
125-144: 클릭 영역이 텍스트에만 제한됨현재
noRippleClickable이AppText에 적용되어 있어 클릭 가능 영역이 날짜 텍스트 부분으로 제한됩니다. 사용자 경험을 위해 전체 44dp 셀 영역이 클릭 가능하도록Box에 클릭 핸들러를 적용하는 것이 좋습니다.♻️ 개선 제안
Box( modifier = Modifier .size(44.dp) + .noRippleClickable { onClick() } .then( if (selected) Modifier.border(1.dp, GrayColor.C500, CircleShape) else Modifier, ), contentAlignment = Alignment.Center, ) { AppText( text = dayOfMonth.toString(), style = AppTextStyle.B1, color = GrayColor.C400, - modifier = - Modifier - .fillMaxSize() - .wrapContentSize(Alignment.Center) - .noRippleClickable { onClick() }, ) }
43-43:today값이 자정 이후에도 갱신되지 않음
remember { LocalDate.now() }는 키 없이 사용되어 앱이 자정을 넘겨 실행 중일 경우 "오늘" 라벨이 갱신되지 않습니다. 일반적인 사용 시나리오에서는 큰 문제가 아니지만, 장시간 앱 사용 시 고려해볼 사항입니다.
34-101: Preview Composable 미제공재사용 가능한 UI 컴포넌트인
WeeklyCalendar에 Preview가 없습니다. 다양한 상태(선택된 날짜, 오늘 날짜 포함 여부 등)에 대한 Preview를 추가하면 컴포넌트 검증이 용이해집니다.core/ui/src/main/java/com/twix/ui/base/BaseViewModel.kt (2)
104-109: 에러 로깅 상세도 검토 제안
handleError에서 전체 스택 트레이스(stackTraceToString())를 로깅하고 있습니다. 개발 단계에서는 유용하지만, 프로덕션 환경에서는 로그 볼륨이 과도해질 수 있습니다. 환경에 따라 로깅 상세도를 조절하거나, Crash Reporting 도구(예: Firebase Crashlytics)로 전달하는 방식을 고려해보시는 것은 어떨까요?
30-30: Channel 용량에 대한 고려
Channel.BUFFERED는 기본적으로 64개의 버퍼를 가집니다. 대부분의 UI 인터랙션에는 충분하지만, 빠른 연속 입력 시나리오(예: 빠른 버튼 연타)에서 버퍼 오버플로우가 발생할 수 있습니다. 현재trySend실패 시 로깅을 하고 있어 추적은 가능하지만, 필요에 따라Channel.UNLIMITED나 커스텀 용량을 고려해볼 수 있습니다.
chanho0908
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생했어 현수아
궁금한게 몇 가지 있어서 리뷰 남겼으니 확인해줘 ! 🥊
| .clickable( | ||
| indication = null, | ||
| onClick = onClick, | ||
| interactionSource = remember { MutableInteractionSource() }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리플 효과를 제거하려는 의도였다면 현수가 만든 noRippleClickable을 사용하는건 어떨까 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
적용했습니다!
| } | ||
|
|
||
| private fun shiftWeek(offset: Long) { | ||
| val newReference = currentState.referenceDate.plusDays(offset * 7) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 7이라는 매직 넘버의 의미를 좀 더 명확히 표현할 수 있도록 상수화 하는건 어떨까 ?
| HomeIntent.NextWeek -> shiftWeek(1) | ||
| HomeIntent.PreviousWeek -> shiftWeek(-1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 1과 -1이라는 숫자가 "다음주로 이동한다" "지난주로 이동한다" 처럼 명확한
의미를 전달할 수 있도록 만들면 코드를 조금 더 읽기 쉬워질거 같다고 생각하는데 어떻게 생각해 ? 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오늘로 돌아가는 기능 추가하면서 같이 수정했습니다!
| .clickable( | ||
| indication = null, | ||
| interactionSource = interactionSource, | ||
| onClick = onClick, | ||
| ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noRippleClickable을 사용할 수 있을 것 같아 !
| init { | ||
| // Intent 순차 처리 | ||
| viewModelScope.launch { | ||
| intentChannel.consumeAsFlow().collect { intent -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
channel에 consumeAsFlow와 receiveAsFlow가 있는걸로 아는데
여기선 어떤 기준으로 consumeAsFlow를 사용했는지 궁금해 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그냥 습관적으로 consumeAsFlow를 썼는데 onCleared에서 close를 호출하고 있으니까 receiveAsFlow를 사용하는 게 더 맞는 거 같아요 수정했습니다!
| * UI에서 Intent를 발생시키는 메서드 | ||
| * */ | ||
| fun dispatch(intent: I) { | ||
| val result = intentChannel.trySend(intent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요기두 send와 trySend가 있는걸로 아는데 어떤 기준으로 선택했을까 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dispatch는 UI에서 바로 호출되는 동기 함수로 두는 게 맞다고 생각했어요. send는 suspend라서 dispatch 내부에서 viewModelScope.launch로 한 번 감싸거나, 호출부가 코루틴 컨텍스트에서 호출되도록 강제해야 하는데, 단순 UI 이벤트 처리에서는 불필요하게 복잡해질 수 있다고 판단했습니다.
| private fun TopBarButton( | ||
| icon: Painter, | ||
| contentDescription: String, | ||
| onClick: () -> Unit, | ||
| ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
공식 문서에 따르면 Painter는 stable 하지 않다고 안내하고 있는 것으로 알고있는데,
imageVector를 사용하는건 어떨까 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 확인해봤는데, 우리 프로젝트는 Material 아이콘보다 Figma에서 내려받아서 관리하는 아이콘이 더 많고, PNG도 섞여 있어서 ImageVector로 통일하기엔 전제가 맞지 않는 것 같아요.
그래서 TopBarButton은 Painter 대신 @DrawableRes iconRes: Int를 받도록 변경하고, 내부에서 painterResource(iconRes)로 로드하는 방식으로 수정하면 될 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@feature/main/src/main/java/com/twix/home/HomeScreen.kt`:
- Around line 64-68: The HomeTopBar usage currently passes empty lambdas for
onNotificationClick and onSettingClick (and similarly empty handlers around
lines 84-90), leaving the UI non-responsive; update the HomeScreen composable to
accept callback parameters (e.g., onNotificationClick: () -> Unit,
onSettingClick: () -> Unit, onAddClick: () -> Unit) and wire those to the
ViewModel intents or navigation actions, or if not ready, pass a disabled state
or a TODO handler that shows a Snackbar/Log to indicate the action is not
implemented; locate and update the HomeTopBar call site and the add-button
handler (the components using onNotificationClick/onSettingClick and the add
action) to forward the injected callbacks instead of empty lambdas.
- Around line 5-6: Remove the unused imports causing KtLint failure: delete the
androidx.compose.foundation.clickable and
androidx.compose.foundation.interaction.MutableInteractionSource imports (and
the other unused import flagged at line 17) if they are not actually used in
HomeScreen.kt; alternatively, if they are intended, update the code to reference
these symbols (e.g., use clickable or MutableInteractionSource in the
composable) so the imports are required.
🧹 Nitpick comments (1)
feature/main/src/main/java/com/twix/home/HomeScreen.kt (1)
46-115: Preview 추가로 UI 회귀 확인을 쉽게 해주세요.
왜 문제인지: Preview가 없으면 레이아웃/테마 변경 시 빠른 확인이 어렵습니다.
어떻게 개선할지:HomeScreen용@Preview를 추가하고 더미HomeUiState를 제공해 간단히 렌더링 확인이 가능하도록 해주세요.
As per coding guidelines, ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@core/ui/src/main/java/com/twix/ui/base/BaseViewModel.kt`:
- Line 10: BaseViewModel.kt currently still imports
kotlinx.coroutines.flow.consumeAsFlow even though code was switched to
receiveAsFlow; remove the unused import to satisfy KtLint. Open the
BaseViewModel file, delete the import line "import
kotlinx.coroutines.flow.consumeAsFlow" (keeping or ensuring "import
kotlinx.coroutines.flow.receiveAsFlow" is present if used), and run the
linter/build to confirm the unused-import warning is resolved.
dogmania
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TopBar 수정하는 김에 오늘 날짜로 되돌리는 버튼도 추가했습니다!
| init { | ||
| // Intent 순차 처리 | ||
| viewModelScope.launch { | ||
| intentChannel.consumeAsFlow().collect { intent -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그냥 습관적으로 consumeAsFlow를 썼는데 onCleared에서 close를 호출하고 있으니까 receiveAsFlow를 사용하는 게 더 맞는 거 같아요 수정했습니다!
| .clickable( | ||
| indication = null, | ||
| onClick = onClick, | ||
| interactionSource = remember { MutableInteractionSource() }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
적용했습니다!
| private fun TopBarButton( | ||
| icon: Painter, | ||
| contentDescription: String, | ||
| onClick: () -> Unit, | ||
| ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 확인해봤는데, 우리 프로젝트는 Material 아이콘보다 Figma에서 내려받아서 관리하는 아이콘이 더 많고, PNG도 섞여 있어서 ImageVector로 통일하기엔 전제가 맞지 않는 것 같아요.
그래서 TopBarButton은 Painter 대신 @DrawableRes iconRes: Int를 받도록 변경하고, 내부에서 painterResource(iconRes)로 로드하는 방식으로 수정하면 될 것 같습니다!
| * UI에서 Intent를 발생시키는 메서드 | ||
| * */ | ||
| fun dispatch(intent: I) { | ||
| val result = intentChannel.trySend(intent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dispatch는 UI에서 바로 호출되는 동기 함수로 두는 게 맞다고 생각했어요. send는 suspend라서 dispatch 내부에서 viewModelScope.launch로 한 번 감싸거나, 호출부가 코루틴 컨텍스트에서 호출되도록 강제해야 하는데, 단순 UI 이벤트 처리에서는 불필요하게 복잡해질 수 있다고 판단했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@core/design-system/src/main/res/drawable/ic_refresh.xml`:
- Around line 6-12: The path element currently hardcodes
android:strokeColor="#858585" which breaks theme/dark-mode contrast; change the
strokeColor to reference a design-system color or theme attribute (e.g.,
`@color/ds_icon_stroke` or ?attr/colorOnSurfaceVariant) in the <path> element
instead of the hex literal, and ensure that the referenced color is defined with
day/night variants (or theme attributes) so dark mode contrast is preserved;
update the <path> android:strokeColor attribute and add the corresponding color
resource(s) or attr definitions (and night resource) to the design system color
set.
In `@core/ui/src/main/java/com/twix/ui/base/BaseViewModel.kt`:
- Around line 78-103: The call to block() in launchResult can throw and bypass
the Result.fold handling, so wrap the suspend block invocation inside a
try/catch that catches Throwable (rethrowing CancellationException) and converts
exceptions into Result.failure(t); then pass that Result into result.fold so
handleError and onError are invoked for both explicit Result failures and thrown
exceptions—update the logic in launchResult (the block invocation and subsequent
result handling) to perform this try/catch→Result.failure transformation.
♻️ Duplicate comments (3)
feature/main/src/main/java/com/twix/home/component/HomeTopBar.kt (1)
104-122:painterResource대신imageVector사용을 고려해 주세요.왜 문제인지: Android 공식 문서에 따르면
Painter는 stable하지 않아 불필요한 recomposition을 유발할 수 있습니다.어떻게 개선할지: 벡터 드로어블의 경우
ImageVector와Icon컴포저블을 사용하면 더 안정적입니다.♻️ 개선 제안
`@Composable` private fun TopBarButton( - `@DrawableRes` iconRes: Int, + imageVector: ImageVector, contentDescription: String, onClick: () -> Unit, ) { Box( modifier = Modifier .size(40.dp) .noRippleClickable(onClick = onClick), contentAlignment = Alignment.Center, ) { - Image( - painter = painterResource(id = iconRes), + Icon( + imageVector = imageVector, contentDescription = contentDescription, ) } }feature/main/src/main/java/com/twix/home/HomeScreen.kt (1)
44-92:HomeScreen의 State Hoisting이 잘 적용되어 있습니다.순수 함수로서 외부에서 상태와 콜백을 주입받아 테스트 가능성과 재사용성이 높습니다.
다만, 몇 가지 개선 사항을 제안드립니다:
빈 람다 처리:
onNotificationClick,onSettingClick,AddGoalButton의onClick이 빈 람다로 전달되고 있습니다. 아직 미구현 상태라면 TODO 주석을 추가하거나, 상위에서 콜백을 주입받아 ViewModel Intent와 연결하는 방식은 어떨까요?Preview 누락: 코딩 가이드라인에 따르면 Preview Composable 제공이 권장됩니다.
HomeScreen에 대한 Preview를 추가하면 UI 개발 시 빠른 피드백을 받을 수 있습니다.♻️ Preview 추가 제안
`@Preview`(showBackground = true) `@Composable` private fun HomeScreenPreview() { TwixTheme { HomeScreen( uiState = HomeUiState(), onSelectDate = {}, onPreviousWeek = {}, onNextWeek = {}, onUpdateVisibleDate = {}, onMoveToToday = {}, ) } }core/ui/src/main/java/com/twix/ui/base/BaseViewModel.kt (1)
49-54: 버퍼 가득참 시 인텐트 유실 가능성, 의도된 동작인가요?
왜 문제가 되냐면 Line 50~52에서trySend실패 시 로그만 남겨 이벤트가 사라질 수 있어 UX가 흔들릴 수 있습니다.
어떻게 개선할지로는, 의도된 설계가 아니라면send를 별도 코루틴에서 시도하거나 버퍼 전략을 명시하는 방식이 있습니다.♻️ 유실 방지를 위한 한 가지 예시
fun dispatch(intent: I) { - val result = intentChannel.trySend(intent) - if (result.isFailure) { - logger.w { "이벤트 유실: $intent, 원인 = ${result.exceptionOrNull()}" } - } + viewModelScope.launch { + try { + intentChannel.send(intent) + } catch (t: Throwable) { + if (t is CancellationException) throw t + logger.w { "이벤트 유실: $intent, 원인 = ${t.message}" } + } + } }Kotlin coroutines Channel trySend 실패 조건과 Channel.BUFFERED 용량/동작
🧹 Nitpick comments (1)
feature/main/src/main/java/com/twix/home/HomeScreen.kt (1)
94-115:AddGoalButton도painterResource대신ImageVector사용을 고려해 주세요.왜 문제인지:
HomeTopBar.kt의TopBarButton과 동일하게Painter가 stable하지 않아 recomposition 최적화에 영향을 줄 수 있습니다.어떻게 개선할지: 벡터 아이콘의 경우
Icon컴포저블과ImageVector를 사용하면 더 안정적입니다.♻️ 개선 제안
`@Composable` private fun AddGoalButton( modifier: Modifier = Modifier, onClick: () -> Unit, ) { Box( modifier = modifier .size(56.dp) .background(GrayColor.C500, CircleShape) .noRippleClickable(onClick = onClick), contentAlignment = Alignment.Center, ) { - Image( - painter = painterResource(R.drawable.ic_plus), - contentDescription = "add goal", - modifier = - Modifier - .size(40.dp), - ) + Icon( + imageVector = ImageVector.vectorResource(R.drawable.ic_plus), + contentDescription = "add goal", + modifier = Modifier.size(40.dp), + tint = Color.Unspecified, // 원본 색상 유지 시 + ) } }
chanho0908
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
큰 작업하느라 고생 많았어 !!
이슈 번호
작업내용
리뷰어에게 추가로 요구하는 사항 (선택)
MainScreen이 컨테이너 역할로 GNB 탭을 관리하고 화면을 분기처리하고 있습니다. 이후에 MainScreen에 토스트 메시지 렌더링 로직과 바텀시트 렌더링을 추가할 예정이라 전체적인 구조를 봐주시면 좋을 것 같아요!
HomeScreen에서는 아직 목표에 대한 DTO가 정의되지 않아서 목표가 비어있는 것을 전제로, 달력 전환과 상단 날짜가 변경되는 로직만 구현해 뒀습니다.